-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Call conduit over HTTP #60
Conversation
6dc74f5
to
82b481c
Compare
cc @cburroughs I think the only thing that's left to satisfy the needs of the "generic conduit" task is to omit the checking for |
I'll have to re-integrate on my local stack, but broadly I think this looks good and in the right direction. |
(Oh and I like how the credentials changes side-step the shared config problem.) |
@jjx can you help review? It's kind of a monster so if you don't have time I can self review, but would like to get some sort of sanity check here. |
This removes much of the reliance on `arc` being installed. Also refactor apply patch into a task -- I realize this should be a separate commit, but all of this stuff is intertwined and I wanted to break it out to make this refactor easier.
.stdout(logger.getStream()) | ||
.cmds(Arrays.asList("git", "submodule", "update", "--init", "--recursive")) | ||
.join(); | ||
DifferentialClient diffClient = new DifferentialClient(diffID, conduitClient); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just initialize this inside the try since it's not used anywhere else.
public HttpUriRequest createRequest(String action, Map<String, String> data) throws UnsupportedEncodingException, ConduitAPIException { | ||
HttpPost post; | ||
try { | ||
post = new HttpPost( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do new URL(String.format("%s/api/%s", baseUrl, action)) instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had that originally, but when testing manually I was running into issues with double-slashes, e.g. "http://phabricator.testing//api/" when the user inputted a trailing slash. Rather than doing manual string munging, I thought I'd just use URL, but I agree that this is awkward. Is there a better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.. I guess this is a mistake proof.
66b4ff5
to
fd71a4f
Compare
e.getMessage(), | ||
query.toString(2))); | ||
} | ||
try { | ||
return response.getJSONObject(diffID); | ||
} catch (JSONException e) { | ||
throw new ArcanistUsageException( | ||
throw new ConduitAPIException( | ||
String.format("Unable to find '%s' key in response: (%s) %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/response/result/
.cmds(Arrays.asList(gitPath, "submodule", "update", "--init", "--recursive")) | ||
.join(); | ||
|
||
List<String> params = new ArrayList<String>(Arrays.asList("--nobranch", "--diff", diffID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arcPatchParams maybe to be more specific?
@jjx I think I addressed all your feedback; ready for another review |
🍏 |
This removes much of the reliance on
arc
being installed.Also refactor apply patch into a task -- I realize this should be a separate
commit, but all of this stuff is intertwined and I wanted to break it out to
make this refactor easier.
Supersedes #55